Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more context and text around ACK frequency #3706

Merged
merged 9 commits into from Jun 1, 2020
Merged

Conversation

janaiyengar
Copy link
Contributor

This PR adds explicit context around the choice of ACK frequency in the draft. It also takes in Bob Briscoe's suggestion in #3529, of calling it a guidance in light of the best information we have.

Also does some editorial cleanup of text blocks that were not in the right sections.

@janaiyengar janaiyengar added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels May 28, 2020
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little too strong in its current form, I think.

forthcoming from the receiver. Therefore, a sender SHOULD ensure that other
frames are sent in addition to PADDING frames to elicit acknowledgments from
the receiver.
The algorithms in {{QUIC-RECOVERY}} are resilient to receivers that do not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to add "generally" to "resilient"? Or, "in principle"? Or "in theory"?

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Comment on lines 3440 to 3444
A receiver determines how frequently to send acknowledgements in response to
ack-eliciting packets. Under normal circumstances, reducing the frequency of
acknowledgement packets can improve connection and endpoint performance in
several ways, such as reducing computational cost at both endpoints, and
improving connection throughput on severely asymmetric links.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to phrase this more as a trade-off. This is concentrated on the benefits of reducing acknowledgments. But the introduction here also needs to recognize that generating acknowledgments in a timely fashion is critical for performance.

several ways, such as reducing computational cost at both endpoints, and
improving connection throughput on severely asymmetric links.

However, there are undesirable consequences to a receiver simply reducing the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you balance the first paragraph out, then you don't need a "however" here. You can just talk in more detail about the advantages of more frequent acknowledgment.

Comment on lines 3447 to 3452
acknowledgement frequency, especially to an arbitrary fixed value. A sender
relies on receipt of acknowledgements to determine the amount of data in flight
and to detect loss; see {{QUIC-RECOVERY}}. Loss detection can be delayed with
late acknowledgements. Window-based congestion controllers, such as the one
defined in {{QUIC-RECOVERY}}, rely on receipt of acknowledgments to increase
their sending rate. Consequently, a connection can suffer performance penalties
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move the general statement at the end of this to the first paragraph, then you can probably shorten this paragraph to:

Endpoints rely on timely acknowledgment to detect loss; see Section X of {{QUIC-RECOVERY}}. Window-based congestion controllers, such as the one in Section X of {{QUIC-RECOVERY}} rely on acknowledgments to manage their sending rate. In both cases, delaying acknowledgment can adversely affect performance.

We don't need the specific dig regarding the "arbitrary fixed value".

Comment on lines 3455 to 3458
Further research and experimentation will yield effective mechanisms to balance
this tradeoff. Meanwhile, this document provides the following guidance:
{{sending-acknowledgements}}, a receiver SHOULD generate an ACK frame for at
least every second ack-eliciting packet. This recommendation is in keeping with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A receiver SHOULD send an ACK frame after receiving at least two ack-eliciting packets. Better knowledge of network conditions or further research and experimentation might suggest alternative acknowledgment strategies with better performance characteristics. This recommendation is general in nature and consistent with recommendations for TCP endpoint behavior {{?RFC5681}}.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
janaiyengar and others added 2 commits May 28, 2020 16:27
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This appears to be much better. And it points in the right direction without being overly prescriptive.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Comment on lines 3453 to 3456
packets. Better knowledge of network conditions or further research and
experimentation might suggest alternative acknowledgment strategies with better
performance characteristics. This recommendation is general in nature and
consistent with recommendations for TCP endpoint behavior {{?RFC5681}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe we can swap the last two sentences, so that "this recommendation" is better bound to the first sentence.

control purposes {{QUIC-RECOVERY}}. Sending only PADDING frames might cause the
sender to become limited by the congestion controller with no acknowledgments
forthcoming from the receiver. Therefore, a sender SHOULD ensure that other
frames are sent periodically in addition to PADDING frames to elicit acknowledgments from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check line length.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be heading in a good direction, thanks for working on it.

Comment on lines 3445 to 3446
Section X of {{QUIC-RECOVERY}}, rely on acknowledgments to manage their sending
rate. In both cases, delaying acknowledgment can adversely affect performance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Section X of {{QUIC-RECOVERY}}, rely on acknowledgments to manage their sending
rate. In both cases, delaying acknowledgment can adversely affect performance.
Section X of {{QUIC-RECOVERY}}, rely on acknowledgments to open up congestion window,
allowing more packets to be sent, as well as increase the congestion window. In both
cases, delaying acknowledgments can adversely affect performance.

Section X of {{QUIC-RECOVERY}}, rely on acknowledgments to manage their sending
rate. In both cases, delaying acknowledgment can adversely affect performance.

On the other hand, reducing the frequency of acknowledgement packets reduces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean ACK-only packets? Otherwise this could be read to include ACK frames bundled with data, which I don't believe is the intent.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Comment on lines 3453 to 3455
packets. Better knowledge of network conditions or further research and
experimentation might suggest alternative acknowledgment strategies with better
performance characteristics. This recommendation is general in nature and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
packets. Better knowledge of network conditions or further research and
experimentation might suggest alternative acknowledgment strategies with better
performance characteristics. This recommendation is general in nature and
packets. Knowledge of how frequently the peer's congestion controller needs feedback,
network conditions or further research and experimentation might suggest alternative
acknowledgment strategies with better performance characteristics. This
recommendation is general in nature and

janaiyengar and others added 2 commits May 28, 2020 22:43
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
@janaiyengar
Copy link
Contributor Author

Thanks @ianswett @martinthomson -- comments incorporated.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

Endpoints rely on timely acknowledgment to detect loss; see Section 5 of
{{QUIC-RECOVERY}}. Window-based congestion controllers, such as the one in
Section 6 of {{QUIC-RECOVERY}}, rely on acknowledgments to manage their sending
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still like to be clearer here and I don't really like "sending rate" in the context of window-based controllers. I made some specific suggestions before. Can you take another look at them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still quite accurate, IMO. Ultimately, increasing the congestion window increases the sending rate. That is the higher order point here -- that reducing ack frequency reduces the throughput of the sender.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to "manage their congestion window"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think that's better, but I also think it doesn't highlight a key point, which is if a receiver delays sending an ACK for Xms, the sender may be blocked from sending for Xms.

Except for slow start, I don't think the congestion window itself is the issue, but rather the available congestion window.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
janaiyengar and others added 2 commits May 31, 2020 19:32
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
janaiyengar and others added 2 commits May 31, 2020 23:24
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
@janaiyengar janaiyengar merged commit b3ea75a into master Jun 1, 2020
@janaiyengar janaiyengar deleted the jri/ackf branch June 1, 2020 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants